Skip to content

Commit

Permalink
Add topology checks to key chaining
Browse files Browse the repository at this point in the history
If key chaining is on, then we refuse to add super/subspaces that exist in topology.
The reason is that, for now, there are no public keys associated with those namespaces,
and without a public key, we have no way to verify the legitimacy of the incoming
registration request.

If token chaining is off, we still prevent re-registering in the registry any prefix
already registered in topology -- but we don't prevent registration of super/subspaces.
  • Loading branch information
jhiemstrawisc committed Dec 6, 2023
1 parent ea6dcb7 commit fe01754
Show file tree
Hide file tree
Showing 3 changed files with 122 additions and 20 deletions.
105 changes: 88 additions & 17 deletions namespace_registry/client_commands_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ import (
"github.com/stretchr/testify/require"
)

func registryMockup(t *testing.T) *httptest.Server {
issuerTempDir := t.TempDir()
func registryMockup(t *testing.T, testName string) *httptest.Server {
issuerTempDir := t.TempDir() + testName

ikey := filepath.Join(issuerTempDir, "issuer.jwk")
viper.Set("IssuerKey", ikey)
Expand All @@ -59,7 +59,7 @@ func registryMockup(t *testing.T) *httptest.Server {
func TestServeNamespaceRegistry(t *testing.T) {
viper.Reset()

svr := registryMockup(t)
svr := registryMockup(t, "serveregistry")
defer func() {
ShutdownDB()
svr.CloseClientConnections()
Expand Down Expand Up @@ -135,55 +135,126 @@ func TestServeNamespaceRegistry(t *testing.T) {
viper.Reset()
}

func TestRegistryKeyChaining(t *testing.T) {
func TestRegistryKeyChainingOSDF(t *testing.T) {
viper.Reset()
_ = config.SetPreferredPrefix("OSDF")
// On by default, but just to make things explicit
viper.Set("Registry.RequireKeyChaining", true)
svr := registryMockup(t)

registrySvr := registryMockup(t, "OSDFkeychaining")
topoSvr := topologyMockup(t, []string{"/topo/foo"})
viper.Set("Federation.TopologyNamespaceURL", topoSvr.URL)
err := PopulateTopology()
require.NoError(t, err)

defer func() {
ShutdownDB()
svr.CloseClientConnections()
svr.Close()
registrySvr.CloseClientConnections()
registrySvr.Close()
topoSvr.CloseClientConnections()
topoSvr.Close()
}()

_, err := config.GetIssuerPublicJWKS()
_, err = config.GetIssuerPublicJWKS()
require.NoError(t, err)
privKey, err := config.GetIssuerPrivateJWK()
require.NoError(t, err)

//Test we register /foo/bar with the default key
err = NamespaceRegister(privKey, svr.URL+"/api/v1.0/registry", "", "/foo/bar")
// Start by registering /foo/bar with the default key
err = NamespaceRegister(privKey, registrySvr.URL+"/api/v1.0/registry", "", "/foo/bar")
require.NoError(t, err)

// Perform one test with a subspace and the same key -- should succeed
err = NamespaceRegister(privKey, svr.URL+"/api/v1.0/registry", "", "/foo/bar/test")
err = NamespaceRegister(privKey, registrySvr.URL+"/api/v1.0/registry", "", "/foo/bar/test")
require.NoError(t, err)

// For now, we simply don't allow further super/sub spacing of namespaces from topo, because how
// can we validate via a key if there is none?
err = NamespaceRegister(privKey, registrySvr.URL+"/api/v1.0/registry", "", "/topo/foo/bar")
require.Error(t, err)

err = NamespaceRegister(privKey, registrySvr.URL+"/api/v1.0/registry", "", "/topo")
require.Error(t, err)

// Now we create a new key and try to use it to register a super/sub space. These shouldn't succeed
viper.Set("IssuerKey", t.TempDir()+"/keychaining")
_, err = config.GetIssuerPublicJWKS()
require.NoError(t, err)
privKey, err = config.GetIssuerPrivateJWK()
require.NoError(t, err)

err = NamespaceRegister(privKey, svr.URL+"/api/v1.0/registry", "", "/foo/bar/baz")
err = NamespaceRegister(privKey, registrySvr.URL+"/api/v1.0/registry", "", "/foo/bar/baz")
require.ErrorContains(t, err, "Cannot register a namespace that is suffixed or prefixed")

err = NamespaceRegister(privKey, svr.URL+"/api/v1.0/registry", "", "/foo")
err = NamespaceRegister(privKey, registrySvr.URL+"/api/v1.0/registry", "", "/foo")
require.ErrorContains(t, err, "Cannot register a namespace that is suffixed or prefixed")

// Make sure we can register things similar but distinct in prefix and suffix
err = NamespaceRegister(privKey, svr.URL+"/api/v1.0/registry", "", "/fo")
err = NamespaceRegister(privKey, registrySvr.URL+"/api/v1.0/registry", "", "/fo")
require.NoError(t, err)
err = NamespaceRegister(privKey, registrySvr.URL+"/api/v1.0/registry", "", "/foo/barz")
require.NoError(t, err)

// Now turn off token chaining and retry -- no errors should occur
viper.Set("Registry.RequireKeyChaining", false)
err = NamespaceRegister(privKey, registrySvr.URL+"/api/v1.0/registry", "", "/foo/bar/baz")
require.NoError(t, err)

err = NamespaceRegister(privKey, registrySvr.URL+"/api/v1.0/registry", "", "/foo")
require.NoError(t, err)

// Finally, test with one value for topo
err = NamespaceRegister(privKey, registrySvr.URL+"/api/v1.0/registry", "", "/topo")
require.NoError(t, err)

config.SetPreferredPrefix("pelican")
viper.Reset()
}

func TestRegistryKeyChaining(t *testing.T) {
viper.Reset()
// On by default, but just to make things explicit
viper.Set("Registry.RequireKeyChaining", true)

registrySvr := registryMockup(t, "keychaining")
defer func() {
ShutdownDB()
registrySvr.CloseClientConnections()
registrySvr.Close()
}()

_, err := config.GetIssuerPublicJWKS()
require.NoError(t, err)
privKey, err := config.GetIssuerPrivateJWK()
require.NoError(t, err)

// Start by registering /foo/bar with the default key
err = NamespaceRegister(privKey, registrySvr.URL+"/api/v1.0/registry", "", "/foo/bar")
require.NoError(t, err)
err = NamespaceRegister(privKey, svr.URL+"/api/v1.0/registry", "", "/foo/barz")

// Perform one test with a subspace and the same key -- should succeed
err = NamespaceRegister(privKey, registrySvr.URL+"/api/v1.0/registry", "", "/foo/bar/test")
require.NoError(t, err)

// Now we create a new key and try to use it to register a super/sub space. These shouldn't succeed
viper.Set("IssuerKey", t.TempDir()+"/keychaining")
_, err = config.GetIssuerPublicJWKS()
require.NoError(t, err)
privKey, err = config.GetIssuerPrivateJWK()
require.NoError(t, err)

err = NamespaceRegister(privKey, registrySvr.URL+"/api/v1.0/registry", "", "/foo/bar/baz")
require.ErrorContains(t, err, "Cannot register a namespace that is suffixed or prefixed")

err = NamespaceRegister(privKey, registrySvr.URL+"/api/v1.0/registry", "", "/foo")
require.ErrorContains(t, err, "Cannot register a namespace that is suffixed or prefixed")

// Now turn off token chaining and retry -- no errors should occur
viper.Set("Registry.RequireKeyChaining", false)
err = NamespaceRegister(privKey, svr.URL+"/api/v1.0/registry", "", "/foo/bar/baz")
err = NamespaceRegister(privKey, registrySvr.URL+"/api/v1.0/registry", "", "/foo/bar/baz")
require.NoError(t, err)

err = NamespaceRegister(privKey, svr.URL+"/api/v1.0/registry", "", "/foo")
err = NamespaceRegister(privKey, registrySvr.URL+"/api/v1.0/registry", "", "/foo")
require.NoError(t, err)

viper.Reset()
Expand Down
7 changes: 6 additions & 1 deletion namespace_registry/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -295,12 +295,17 @@ func keySignChallengeCommit(ctx *gin.Context, data *registrationData, action str
}

if param.Registry_RequireKeyChaining.GetBool() {
superspaces, subspaces, err := namespaceSupSubChecks(data.Prefix)
superspaces, subspaces, inTopo, err := namespaceSupSubChecks(data.Prefix)
if err != nil {
log.Errorf("Failed to check if namespace suffixes or prefixes another registered namespace: %v", err)
return errors.Wrap(err, "Server encountered an error checking if namespace already exists")
}

// if not in OSDF mode, this will be false
if inTopo {
_ = ctx.AbortWithError(403, errors.New("Cannot register a super or subspace of a namespace already registered in topology"))
return errors.New("Cannot register a super or subspace of a namespace already registered in topology")
}
// If we make the assumption that namespace prefixes are heirarchical, eg that the owner of /foo should own
// everything under /foo (/foo/bar, /foo/baz, etc), then it makes sense to check for superspaces first. If any
// superspace is found, they logically "own" the incoming namespace.
Expand Down
30 changes: 28 additions & 2 deletions namespace_registry/registry_db.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,11 +112,38 @@ func namespaceExists(prefix string) (bool, error) {
return found, nil
}

func namespaceSupSubChecks(prefix string) (superspaces []string, subspaces []string, err error) {
func namespaceSupSubChecks(prefix string) (superspaces []string, subspaces []string, inTopo bool, err error) {
// The very first thing we do is check if there's a match in topo -- if there is, for now
// we simply refuse to allow registration of a superspace or a subspace, assuming the registrant
// has to go through topology
if config.GetPreferredPrefix() == "OSDF" {
topoSuperSubQuery := `
SELECT prefix FROM topology WHERE (? || '/') LIKE (prefix || '/%')
UNION
SELECT prefix FROM topology WHERE (prefix || '/') LIKE (? || '/%')
`
args := []interface{}{prefix, prefix}
topoSuperSubResults, tmpErr := db.Query(topoSuperSubQuery, args...)
if tmpErr != nil {
err = tmpErr
return
}
defer topoSuperSubResults.Close()

for topoSuperSubResults.Next() {
// if we make it here, there was a match -- it's a trap!
inTopo = true
return
}
topoSuperSubResults.Close()
}

// Check if any registered namespaces already superspace the incoming namespace,
// eg if /foo is already registered, this will be true for an incoming /foo/bar because
// /foo is logically above /foo/bar (according to my logic, anyway)
superspaceQuery := `SELECT prefix FROM namespace WHERE (? || '/') LIKE (prefix || '/%')`
subspaceQuery := `SELECT prefix FROM namespace WHERE (prefix || '/') LIKE (? || '/%')`

superspaceResults, err := db.Query(superspaceQuery, prefix)
if err != nil {
return
Expand All @@ -133,7 +160,6 @@ func namespaceSupSubChecks(prefix string) (superspaces []string, subspaces []str
// Check if any registered namespaces already subspace the incoming namespace,
// eg if /foo/bar is already registered, this will be true for an incoming /foo because
// /foo/bar is logically below /foo
subspaceQuery := `SELECT prefix FROM namespace WHERE (prefix || '/') LIKE (? || '/%')`
subspaceResults, err := db.Query(subspaceQuery, prefix)
if err != nil {
return
Expand Down

0 comments on commit fe01754

Please sign in to comment.