admin: normalize request path in remote admin access-control check (defense-in-depth) (#7910)
* admin: normalize request path in remote admin access-control check
Co-authored-by: atlarix-agent <agent@atlarix.dev>
* admin: fix empty allowedPath regression and dead code in path normalization
path.Clean("") returns ".", so cleaning allowedPath unconditionally
silently broke the allow-all behavior when Paths: [""] is configured.
Short-circuit the empty case before cleaning to preserve that behavior.
Also remove the dead strings.HasSuffix(allowedPath, "/") branch —
after path.Clean the path never has a trailing slash, so the unified
reqPath == allowedPath || HasPrefix(reqPath, allowedPath+"/") form
covers exact match, subpath boundary, and trailing-slash requests.
Co-authored-by: atlarix-agent <agent@atlarix.dev>
Co-authored-by: iabdullah215 <muhammadabdullah8040@gmail.com>
* admin: validate non-canonical configured paths at provisioning
path.Clean(allowedPath) silently broadens misconfigured values like
// or /.. into /, which grants unintended access to all endpoints.
Reject non-canonical paths during provisioning in
replaceRemoteAdminServer so misconfigurations fail fast with a
clear error. The path.Clean in adminPathAllowed remains as
defense-in-depth but is now a safe no-op on validated inputs.
Co-authored-by: atlarix-agent <agent@atlarix.dev>
* admin: validate non-canonical configured paths at provisioning
path.Clean(allowedPath) silently broadens misconfigured values like
// or /.. into /, which grants unintended access to all endpoints.
Reject non-canonical paths during provisioning in
replaceRemoteAdminServer so misconfigurations fail fast with a
clear error. The path.Clean in adminPathAllowed remains as
defense-in-depth but is now a safe no-op on validated inputs.
Co-authored-by: atlarix-agent <agent@atlarix.dev>
* admin: fix TrimRight → TrimSuffix in provisioning path validation
strings.TrimRight strips all trailing slashes, so a configured path
like /foo// passed validation (both slashes trimmed to /foo matching
path.Clean output) but was silently broadened to /foo at runtime.
Use strings.TrimSuffix instead, which removes exactly one trailing
slash — the only form the exemption was meant to allow (users write
/pki/ca/prod/ meaning the /pki/ca/prod scope).
Also update the // test case: with TrimSuffix, // is just / + one
trailing slash, which is valid under the exemption. Add a new test
for /foo// (double trailing slashes → wantErr: true).
Co-authored-by: atlarix-agent <agent@atlarix.dev>
* admin: reject non-canonical root permission path
* admin: preserve trailing-slash permission semantics
---------
Co-authored-by: atlarix-agent <agent@atlarix.dev>
Co-authored-by: iabdullah215 <muhammadabdullah8040@gmail.com>
Co-authored-by: Zen Dodd <mail@steadytao.com>
pull/7952/head^2
Amariah Abishai
GitHub
parent
39af0aec31
commit
d6637934e8
28
admin.go
28
admin.go
|
|
@ -554,6 +554,18 @@ func replaceRemoteAdminServer(ctx Context, cfg *Config) error {
|
|||
accessControl.publicKeys = append(accessControl.publicKeys, cert.PublicKey)
|
||||
clientCertPool.AddCert(cert)
|
||||
}
|
||||
for j, perm := range accessControl.Permissions {
|
||||
for _, permPath := range perm.Paths {
|
||||
if permPath == "" || permPath == "/" {
|
||||
continue
|
||||
}
|
||||
cleanPath := path.Clean(permPath)
|
||||
hasCanonicalTrailingSlash := cleanPath != "/" && strings.TrimSuffix(permPath, "/") == cleanPath
|
||||
if cleanPath != permPath && !hasCanonicalTrailingSlash {
|
||||
return fmt.Errorf("access control %d permission %d: path %q is not canonical (did you mean %q?)", i, j, permPath, cleanPath)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// create TLS config that will enforce mutual authentication
|
||||
|
|
@ -723,14 +735,18 @@ func (remote RemoteAdmin) enforceAccessControls(r *http.Request) error {
|
|||
}
|
||||
|
||||
func adminPathAllowed(reqPath, allowedPath string) bool {
|
||||
if allowedPath == "" || allowedPath == "/" {
|
||||
return strings.HasPrefix(reqPath, allowedPath)
|
||||
}
|
||||
if reqPath == allowedPath {
|
||||
reqPathHadTrailingSlash := strings.HasSuffix(reqPath, "/")
|
||||
reqPath = path.Clean(reqPath)
|
||||
if allowedPath == "" {
|
||||
return true
|
||||
}
|
||||
if strings.HasSuffix(allowedPath, "/") {
|
||||
return strings.HasPrefix(reqPath, allowedPath)
|
||||
allowedPathHadTrailingSlash := allowedPath != "/" && strings.HasSuffix(allowedPath, "/")
|
||||
allowedPath = path.Clean(allowedPath)
|
||||
if allowedPath == "/" {
|
||||
return true
|
||||
}
|
||||
if reqPath == allowedPath {
|
||||
return !allowedPathHadTrailingSlash || reqPathHadTrailingSlash
|
||||
}
|
||||
return strings.HasPrefix(reqPath, allowedPath+"/")
|
||||
}
|
||||
|
|
|
|||
144
admin_test.go
144
admin_test.go
|
|
@ -684,6 +684,18 @@ func TestRemoteAdminAccessControlPathSegmentMatching(t *testing.T) {
|
|||
requestPath: "/pki/ca/prod/certificates",
|
||||
wantErr: false,
|
||||
},
|
||||
{
|
||||
name: "trailing slash scope excludes exact path without slash",
|
||||
allowedPath: "/pki/ca/prod/",
|
||||
requestPath: "/pki/ca/prod",
|
||||
wantErr: true,
|
||||
},
|
||||
{
|
||||
name: "trailing slash scope includes exact path with slash",
|
||||
allowedPath: "/pki/ca/prod/",
|
||||
requestPath: "/pki/ca/prod/",
|
||||
wantErr: false,
|
||||
},
|
||||
{
|
||||
name: "sibling with shared prefix",
|
||||
allowedPath: "/pki/ca/prod",
|
||||
|
|
@ -702,6 +714,48 @@ func TestRemoteAdminAccessControlPathSegmentMatching(t *testing.T) {
|
|||
requestPath: "/pki/ca/prod",
|
||||
wantErr: false,
|
||||
},
|
||||
{
|
||||
name: "empty path preserves allow all",
|
||||
allowedPath: "",
|
||||
requestPath: "/pki/ca/prod",
|
||||
wantErr: false,
|
||||
},
|
||||
{
|
||||
name: "dot-dot traversal out of scope",
|
||||
allowedPath: "/pki/ca/prod",
|
||||
requestPath: "/pki/ca/prod/../../../../load",
|
||||
wantErr: true,
|
||||
},
|
||||
{
|
||||
name: "dot-dot traversal out of trailing slash scope",
|
||||
allowedPath: "/pki/ca/prod/",
|
||||
requestPath: "/pki/ca/prod/../../../../load",
|
||||
wantErr: true,
|
||||
},
|
||||
{
|
||||
name: "encoded traversal (net/url decodes %2e to . before path.Clean resolves ..)",
|
||||
allowedPath: "/pki/ca/prod",
|
||||
requestPath: "/pki/ca/prod/%2e%2e/load",
|
||||
wantErr: true,
|
||||
},
|
||||
{
|
||||
name: "sibling traversal",
|
||||
allowedPath: "/pki/ca/prod",
|
||||
requestPath: "/pki/ca/prod/../staging",
|
||||
wantErr: true,
|
||||
},
|
||||
{
|
||||
name: "collapsed slashes",
|
||||
allowedPath: "/pki/ca/prod",
|
||||
requestPath: "/pki//ca/prod",
|
||||
wantErr: false,
|
||||
},
|
||||
{
|
||||
name: "exact request with trailing slash",
|
||||
allowedPath: "/pki/ca/prod",
|
||||
requestPath: "/pki/ca/prod/",
|
||||
wantErr: false,
|
||||
},
|
||||
}
|
||||
|
||||
for i, test := range tests {
|
||||
|
|
@ -839,6 +893,96 @@ vq+SH04xKhtFudVBAQ==`
|
|||
},
|
||||
wantErr: true,
|
||||
},
|
||||
{
|
||||
name: "non-canonical root path with trailing slash",
|
||||
cfg: &Config{
|
||||
Admin: &AdminConfig{
|
||||
Identity: &IdentityConfig{},
|
||||
Remote: &RemoteAdmin{
|
||||
Listen: "localhost:2021",
|
||||
AccessControl: []*AdminAccess{
|
||||
{
|
||||
PublicKeys: []string{testCert},
|
||||
Permissions: []AdminPermissions{{Methods: []string{"GET"}, Paths: []string{"//"}}},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
wantErr: true,
|
||||
},
|
||||
{
|
||||
name: "canonical subpath with trailing slash",
|
||||
cfg: &Config{
|
||||
Admin: &AdminConfig{
|
||||
Identity: &IdentityConfig{},
|
||||
Remote: &RemoteAdmin{
|
||||
Listen: "localhost:2021",
|
||||
AccessControl: []*AdminAccess{
|
||||
{
|
||||
PublicKeys: []string{testCert},
|
||||
Permissions: []AdminPermissions{{Methods: []string{"GET"}, Paths: []string{"/foo/"}}},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
wantErr: false,
|
||||
},
|
||||
{
|
||||
name: "non-canonical path /..",
|
||||
cfg: &Config{
|
||||
Admin: &AdminConfig{
|
||||
Identity: &IdentityConfig{},
|
||||
Remote: &RemoteAdmin{
|
||||
Listen: "localhost:2021",
|
||||
AccessControl: []*AdminAccess{
|
||||
{
|
||||
PublicKeys: []string{testCert},
|
||||
Permissions: []AdminPermissions{{Methods: []string{"GET"}, Paths: []string{"/.."}}},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
wantErr: true,
|
||||
},
|
||||
{
|
||||
name: "non-canonical path with double slashes",
|
||||
cfg: &Config{
|
||||
Admin: &AdminConfig{
|
||||
Identity: &IdentityConfig{},
|
||||
Remote: &RemoteAdmin{
|
||||
Listen: "localhost:2021",
|
||||
AccessControl: []*AdminAccess{
|
||||
{
|
||||
PublicKeys: []string{testCert},
|
||||
Permissions: []AdminPermissions{{Methods: []string{"GET"}, Paths: []string{"/foo//bar"}}},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
wantErr: true,
|
||||
},
|
||||
{
|
||||
name: "non-canonical path with double trailing slashes",
|
||||
cfg: &Config{
|
||||
Admin: &AdminConfig{
|
||||
Identity: &IdentityConfig{},
|
||||
Remote: &RemoteAdmin{
|
||||
Listen: "localhost:2021",
|
||||
AccessControl: []*AdminAccess{
|
||||
{
|
||||
PublicKeys: []string{testCert},
|
||||
Permissions: []AdminPermissions{{Methods: []string{"GET"}, Paths: []string{"/foo//"}}},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
wantErr: true,
|
||||
},
|
||||
}
|
||||
|
||||
for _, test := range tests {
|
||||
|
|
|
|||
Loading…
Reference in New Issue