Skip to content

Added use of acceptable_authorization to origin scitokens files #2075

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
turetske wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Added use of acceptable_authorization to origin scitokens files #2075

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 32 additions & 8 deletions xrootd/authorization.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,14 +61,15 @@ type (

// Per-issuer configuration
Issuer struct {
Name string
Issuer string
BasePaths []string
RestrictedPaths []string
MapSubject bool
DefaultUser string
UsernameClaim string
NameMapfile string
Name string
Issuer string
BasePaths []string
RestrictedPaths []string
MapSubject bool
DefaultUser string
UsernameClaim string
NameMapfile string
AcceptableAuthorization string
}

// Top-level configuration object for the template
Expand Down Expand Up @@ -479,6 +480,16 @@ func GenerateOriginIssuer(exportedPaths []string) (issuer Issuer, err error) {
issuer.DefaultUser = param.Origin_ScitokensDefaultUser.GetString()
issuer.UsernameClaim = param.Origin_ScitokensUsernameClaim.GetString()

// This will be set to true if PublicReads are enabled as well and is slightly redundant
// as then a token wouldn't be needed for reads, but doesn't actually change the functionality
// The default if the acceptable authorization isn't set is "all", so we only care if
// Reads is Enabled without Writes or vice versa
if param.Origin_EnableReads.GetBool() && !param.Origin_EnableWrites.GetBool() {
issuer.AcceptableAuthorization = "read"
Copy link
Member

@jhiemstrawisc jhiemstrawisc Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you define "read" and "write" as constants or types?

} else if param.Origin_EnableWrites.GetBool() && !param.Origin_EnableReads.GetBool() {
issuer.AcceptableAuthorization = "write"
}
Comment on lines +483 to +491
Copy link
Member

@jhiemstrawisc jhiemstrawisc Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm interpreting this overall diff correctly, we're setting the issuer's acceptable authorization in the scitokens config per issuer. That's not quite the same as enforcing per-namespace capabilities, especially once #2103 is merged, which allows us to define the prefix<-->issuer relationship as a many-to-many.

Once that PR is in, I think we'll need to change the approach in this section to iterate through each export and determine what the individual issuer(s) for that export should support. That gets us closer to what we want, but still not all the way. Consider a trickier configuration like:

Origin:
  Exports:
    - FederationPrefix: /foo1
      StoragePrefix: /bar1
      Capabilities: ["Writes"]
      IssuerUrls: ["https://issuer1.com"]
    - FederationPrefix: /foo2
      StoragePrefix: /bar2
      Capabilities: ["Reads"]
      IssuerUrls: ["https://issuer1.com"]

In this case, two namespaces define conflicting capabilities but use the same issuer URL. My understanding is this requires us to set all as the acceptable_authorization because otherwise we'd reject tokens for one of the namespaces. But setting all means we'll accept tokens with the wrong scopes for both namespaces. And I don't think we can give two issuers different names for the two different prefixes while having them use the same issuer URL, because XRootD picks up the first URL match and ignores the rest.

Any ideas how we can work around this?

Copy link
Collaborator Author

@turetske turetske Apr 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we just have to use the "all", which is what's happening now. There isn't a way to adjust this without a greater change to the xrootd code. This is already not as granular as we'd like (no difference between write/create/modify, etc.) so I think we just need to take what we can get for now.


return
}

Expand Down Expand Up @@ -529,6 +540,17 @@ func makeSciTokensCfg() (cfg ScitokensCfg, err error) {
return cfg, nil
}

func resolveAuthorization(auth1 string, auth2 string) string {
if auth1 == auth2 {
return auth1
} else {
// Either one is read and the other is write, in which case we want the default of
// all, or one is all and the other is read/write, and we'd still want it to be the default
// of all, which is equivalent to the empty string
return ""
Copy link
Member

@jhiemstrawisc jhiemstrawisc Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm more in favor of using "all" in this case instead of relying on the empty string's default behavior. That should make it a bit easier for Fabio to understand the configs we generate without having to find comments in source code.

}
}

// Writes out the server's scitokens.cfg configuration
func EmitScitokensConfig(server server_structs.XRootDServer) error {
if originServer, ok := server.(*origin.OriginServer); ok {
Expand Down Expand Up @@ -577,6 +599,7 @@ func WriteOriginScitokensConfig(authedPaths []string) error {
if val, ok := cfg.IssuerMap[issuer.Issuer]; ok {
val.BasePaths = append(val.BasePaths, issuer.BasePaths...)
val.Name += " and " + issuer.Name
val.AcceptableAuthorization = resolveAuthorization(val.AcceptableAuthorization, issuer.AcceptableAuthorization)
cfg.IssuerMap[issuer.Issuer] = val
} else {
cfg.IssuerMap[issuer.Issuer] = issuer
Expand All @@ -589,6 +612,7 @@ func WriteOriginScitokensConfig(authedPaths []string) error {
if val, ok := cfg.IssuerMap[issuer.Issuer]; ok {
val.BasePaths = append(val.BasePaths, issuer.BasePaths...)
val.Name += " and " + issuer.Name
val.AcceptableAuthorization = resolveAuthorization(val.AcceptableAuthorization, issuer.AcceptableAuthorization)
cfg.IssuerMap[issuer.Issuer] = val
} else {
cfg.IssuerMap[issuer.Issuer] = issuer
Expand Down
115 changes: 88 additions & 27 deletions xrootd/authorization_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
"os"
"path/filepath"
"reflect"
"slices"
"strings"
"testing"

Expand Down Expand Up @@ -77,6 +78,15 @@ var (
//go:embed resources/osdf-authfile
authfileOutput string

//go:embed resources/test-scitokens-origin-read.cfg
scitokensOriginReadOutput string

//go:embed resources/test-scitokens-origin-write.cfg
scitokensOriginWriteOutput string

//go:embed resources/test-scitokens-origin-read-write.cfg
scitokensOriginReadWriteOutput string

sampleMultilineOutput = `foo \
bar
baz
Expand Down Expand Up @@ -556,6 +566,7 @@ func TestGenerateConfig(t *testing.T) {
viper.Set("Origin.ScitokensMapSubject", true)
viper.Set("Origin.Port", 8443)
viper.Set("Server.WebPort", 8443)
viper.Set("Origin.EnableReads", true)
config.InitConfigDir(viper.GetViper())
err = config.InitServer(ctx, server_structs.OriginType)
require.NoError(t, err)
Expand All @@ -568,6 +579,7 @@ func TestGenerateConfig(t *testing.T) {
assert.Equal(t, "/another/exported/path", issuer.BasePaths[1])
assert.Equal(t, "user1", issuer.DefaultUser)
assert.True(t, issuer.MapSubject)
assert.Equal(t, "read", issuer.AcceptableAuthorization)
}

func TestWriteOriginAuthFiles(t *testing.T) {
Expand Down Expand Up @@ -727,37 +739,86 @@ func TestWriteCacheAuthFiles(t *testing.T) {
}

func TestWriteOriginScitokensConfig(t *testing.T) {
ctx, cancel, egrp := test_utils.TestContext(context.Background(), t)
defer func() { require.NoError(t, egrp.Wait()) }()
defer cancel()
tests := []struct {
Name string
Capabilities []string
ExpectedCfg string
InputCfg string
}{
{
Name: "OriginSciTokensOnlyReads",
Capabilities: []string{"Reads"},
ExpectedCfg: string(scitokensOriginReadOutput),
InputCfg: "",
},
{
Name: "OriginSciTokensOnlyWrites",
Capabilities: []string{"Writes"},
ExpectedCfg: string(scitokensOriginWriteOutput),
InputCfg: "",
},
{
Name: "OriginSciTokensReadsWrites",
Capabilities: []string{"Reads", "Writes"},
ExpectedCfg: string(scitokensOriginReadWriteOutput),
InputCfg: "",
},
{
Name: "OriginScitokensMergeDiff",
Capabilities: []string{"Reads"},
ExpectedCfg: string(monitoringOutput),
InputCfg: string(toMergeOutput),
},
}
for _, test := range tests {
t.Run(test.Name, func(t *testing.T) {
ctx, cancel, egrp := test_utils.TestContext(context.Background(), t)
defer func() { require.NoError(t, egrp.Wait()) }()
defer cancel()

server_utils.ResetTestState()
dirname := t.TempDir()
os.Setenv("PELICAN_ORIGIN_RUNLOCATION", dirname)
defer os.Unsetenv("PELICAN_ORIGIN_RUNLOCATION")
config_dirname := t.TempDir()
viper.Set("Origin.SelfTest", true)
viper.Set("ConfigDir", config_dirname)
viper.Set("Origin.RunLocation", dirname)
viper.Set("Origin.Port", 8443)
viper.Set("Server.WebPort", 8444)
viper.Set("Server.Hostname", "origin.example.com")
viper.Set(param.Origin_StorageType.GetName(), string(server_structs.OriginStoragePosix))
server_utils.ResetTestState()
defer server_utils.ResetTestState()

err := config.InitServer(ctx, server_structs.OriginType)
require.NoError(t, err)
viper.Set("Origin.Port", 8443)
Copy link
Member

@jhiemstrawisc jhiemstrawisc Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know a lot of this code is going to get hit with merge conflicts so I'm not sure if this comment will be relevant soon, but can you switch any of the viper.Sets you touch to use param.XXX.GetName() for the key (the way it's done in line 786 here)?

viper.Set("Server.WebPort", 8444)
viper.Set("Server.Hostname", "origin.example.com")
viper.Set("Server.IssuerUrl", "https://origin.issuer.com")
viper.Set(param.Origin_StorageType.GetName(), string(server_structs.OriginStoragePosix))

scitokensCfg := param.Xrootd_ScitokensConfig.GetString()
err = config.MkdirAll(filepath.Dir(scitokensCfg), 0755, -1, -1)
require.NoError(t, err)
err = os.WriteFile(scitokensCfg, []byte(toMergeOutput), 0640)
require.NoError(t, err)
dirname := t.TempDir()
os.Setenv("PELICAN_ORIGIN_RUNLOCATION", dirname)
defer os.Unsetenv("PELICAN_ORIGIN_RUNLOCATION")
config_dirname := t.TempDir()

err = WriteOriginScitokensConfig([]string{"/foo/bar"})
require.NoError(t, err)
viper.Set("ConfigDir", config_dirname)
viper.Set("Origin.RunLocation", dirname)

genCfg, err := os.ReadFile(filepath.Join(dirname, "scitokens-origin-generated.cfg"))
require.NoError(t, err)
if slices.Contains(test.Capabilities, "Reads") {
viper.Set("Origin.EnableReads", true)
}
if slices.Contains(test.Capabilities, "Writes") {
viper.Set("Origin.EnableWrites", true)
}

assert.Equal(t, string(monitoringOutput), string(genCfg))
err := config.InitServer(ctx, server_structs.OriginType)
require.NoError(t, err)

if test.InputCfg != "" {
viper.Set("Server.IssuerUrl", "")
scitokensCfg := param.Xrootd_ScitokensConfig.GetString()
err = config.MkdirAll(filepath.Dir(scitokensCfg), 0755, -1, -1)
require.NoError(t, err)
err = os.WriteFile(scitokensCfg, []byte(test.InputCfg), 0640)
require.NoError(t, err)
}

err = WriteOriginScitokensConfig([]string{"/foo/bar"})
require.NoError(t, err)

genCfg, err := os.ReadFile(filepath.Join(dirname, "scitokens-origin-generated.cfg"))
require.NoError(t, err)

assert.Equal(t, test.ExpectedCfg, string(genCfg))
})
}
}
3 changes: 3 additions & 0 deletions xrootd/resources/scitokens.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ name_mapfile = {{.NameMapfile}}
{{- if .UsernameClaim}}
username_claim = {{.UsernameClaim}}
{{- end}}
{{- if .AcceptableAuthorization}}
acceptable_authorization = {{.AcceptableAuthorization}}
{{- end}}

{{end -}}
# End of config
34 changes: 34 additions & 0 deletions xrootd/resources/test-scitokens-origin-read-write.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
#
# Copyright (C) 2024, Pelican Project, Morgridge Institute for Research
#
# Licensed under the Apache License, Version 2.0 (the "License"); you
# may not use this file except in compliance with the License. You may
# obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#

#
# This is a generated configuration file -- DO NOT HAND EDIT.
# It will be overwritten on the next startup of pelican.
#

[Global]
audience_json = ["https://origin.example.com:8443"]

[Issuer Built-in Monitoring]
issuer = https://origin.example.com:8444
base_path = /pelican/monitoring
default_user = xrootd

[Issuer Origin]
issuer = https://origin.issuer.com
base_path = /foo/bar

# End of config
35 changes: 35 additions & 0 deletions xrootd/resources/test-scitokens-origin-read.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
#
# Copyright (C) 2024, Pelican Project, Morgridge Institute for Research
Copy link
Member

@jhiemstrawisc jhiemstrawisc Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're officially changing the scitokens config template, can you update the copyright statements?

#
# Licensed under the Apache License, Version 2.0 (the "License"); you
# may not use this file except in compliance with the License. You may
# obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#

#
# This is a generated configuration file -- DO NOT HAND EDIT.
# It will be overwritten on the next startup of pelican.
#

[Global]
audience_json = ["https://origin.example.com:8443"]

[Issuer Built-in Monitoring]
issuer = https://origin.example.com:8444
base_path = /pelican/monitoring
default_user = xrootd

[Issuer Origin]
issuer = https://origin.issuer.com
base_path = /foo/bar
acceptable_authorization = read

# End of config
35 changes: 35 additions & 0 deletions xrootd/resources/test-scitokens-origin-write.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
#
# Copyright (C) 2024, Pelican Project, Morgridge Institute for Research
#
# Licensed under the Apache License, Version 2.0 (the "License"); you
# may not use this file except in compliance with the License. You may
# obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#

#
# This is a generated configuration file -- DO NOT HAND EDIT.
# It will be overwritten on the next startup of pelican.
#

[Global]
audience_json = ["https://origin.example.com:8443"]

[Issuer Built-in Monitoring]
Copy link
Member

@jhiemstrawisc jhiemstrawisc Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another note that should help out with these tests is that once #2103 is in, it'll be easier to test this by working with the slice of generated Issuers instead of working with the files on disk. Some of what I did in that PR might break assumptions about preserved ordering of issuers/base paths when everything is written to a file, so you won't be able to do a direct equality comparison. And working directly with the slice of issuers prevents us from having to write new scitokens configs like this every time we need to test a new edge case.

issuer = https://origin.example.com:8444
base_path = /pelican/monitoring
default_user = xrootd

[Issuer Origin]
issuer = https://origin.issuer.com
base_path = /foo/bar
acceptable_authorization = write

# End of config
Loading