auth: add server-only ACL #18715

tgross · 2023-10-10T17:39:24Z

The RPC handlers expect to see nil ACL objects whenever ACLs are disabled. By using nil as a sentinel value, we have the risk of nil pointer exceptions and improper handling of nil when returned from our various auth methods that can lead to privilege escalation bugs. This is the second in a series to eliminate the use of nil ACLs as a sentinel value for when ACLs are disabled.

This patch involves creating a new "virtual" ACL object for checking permissions on server operations and a matching AuthenticateServerOnly method for server-only RPCs that can produce that ACL.

Ref: https://github.com/hashicorp/nomad-enterprise/pull/1218
Ref: #18703

The RPC handlers expect to see `nil` ACL objects whenever ACLs are disabled. By using `nil` as a sentinel value, we have the risk of nil pointer exceptions and improper handling of `nil` when returned from our various auth methods that can lead to privilege escalation bugs. This is the second in a series to eliminate the use of `nil` ACLs as a sentinel value for when ACLs are disabled. This patch involves creating a new "virtual" ACL object for checking permissions on server operations and a matching `AuthenticateServerOnly` method for server-only RPCs that can produce that ACL. Ref: hashicorp/nomad-enterprise#1218 Ref: #18703

jrasell

LGTM!

Just a couple of random thoughts or questions that can easily be ignored.

acl/acl.go

nomad/auth/auth.go

jrasell · 2023-10-11T07:36:44Z

acl/acl.go

+	// The attribute below is avirtual policy that we never expose directly to
+	// the end user
+	server   string
+	isLeader bool


Just checking my logic that currently this value will always be false, and this is expected?

Yeah there's a LeaderACL object in nomad/structs that'll be used in one of the following PRs. My intent was not to have anything in these PRs that wasn't yet in use to avoid confusion, but I missed this one.

Co-authored-by: James Rasell <[email protected]>

lgfa29

Just some minor questions, but nice refactoring!

lgfa29 · 2023-10-11T04:16:33Z

nomad/auth/auth.go

+// validateCertificateForName returns true if the certificate is valid
+// for the given domain name.
+func validateCertificateForName(cert *x509.Certificate, expectedName string) (bool, error) {
+	if cert == nil {
+		return false, nil
+	}
+
+	validNames := []string{cert.Subject.CommonName}
+	validNames = append(validNames, cert.DNSNames...)
+	for _, valid := range validNames {
+		if expectedName == valid {
+			return true, nil
+		}
+	}
+
+	return false, fmt.Errorf("invalid certificate, %s not in %s",
+		expectedName, strings.Join(validNames, ","))
+}


Is the idea for this function to replace ctx.ValidateCertificateForName()? Since we have a ctx above we could use it directly?

nomad/nomad/rpc.go

Lines 127 to 148 in 635afee

// ValidateCertificateForName returns true if the RPC context certificate is valid

// for the given domain name.

func (ctx *RPCContext) ValidateCertificateForName(name string) error {

if ctx == nil || !ctx.TLS {

return nil

}

cert := ctx.Certificate()

if cert == nil {

return errors.New("missing certificate information")

}

validNames := []string{cert.Subject.CommonName}

validNames = append(validNames, cert.DNSNames...)

for _, valid := range validNames {

if name == valid {

return nil

}

}

return fmt.Errorf("invalid certificate, %s not in %s", name, strings.Join(validNames, ","))

}

You're seeing a bit of an artifact of breaking this work into multiple PRs. We only need the (*RPCContext).ValidateCertificateForName method to exist because we haven't done the "auth: add client-only ACL" PR.

When we've completed that, the only callers for this logic will exist encapsulated in the nomad/auth package so there's no reason to keep it in nomad/rpc.go anymore. That allows us to have all the code that tests it in the nomad/auth package without having to copy the logic into a ValidateCertificateForName method on the testContext struct (at which point we'd be testing the mock context logic and not the production code).

lgfa29 · 2023-10-11T14:12:35Z

nomad/keyring_endpoint.go

-	// we need to allow both humans with management tokens and
-	// non-leader servers to list keys, in order to support
-	// replication
-	err := validateTLSCertificateLevel(k.srv, k.ctx, tlsCertificateLevelServer)
-	if err != nil {
-		if aclObj, err := k.srv.ResolveACL(args); err != nil {
-			return err
-		} else if aclObj != nil && !aclObj.IsManagement() {
-			return structs.ErrPermissionDenied
-		}
+	if aclObj, err := k.srv.ResolveACL(args); err != nil {
+		return err
+	} else if aclObj != nil && !aclObj.IsManagement() {
+		return structs.ErrPermissionDenied
 	}


When servers call this endpoint do they pass an aclObj?

As it turns out, servers don't call this Keyring.List endpoint at all! I think we originally had this to support keyring replication between leader and followers, but the followers just read the metadata from the state store and then Keyring.Get the actually key material.

The RPC handlers expect to see `nil` ACL objects whenever ACLs are disabled. By using `nil` as a sentinel value, we have the risk of nil pointer exceptions and improper handling of `nil` when returned from our various auth methods that can lead to privilege escalation bugs. This is the third in a series to eliminate the use of `nil` ACLs as a sentinel value for when ACLs are disabled. This patch involves creating a new "virtual" ACL object for checking permissions on client operations and a matching `AuthenticateClientOnly` method for client-only RPCs that can produce that ACL. Unlike the server ACLs PR, this also includes a special case for "legacy" client RPCs where the client was not previously sending the secret as it should (leaning on mTLS only). Those client RPCs were fixed in Nomad 1.6.0, but it'll take a while before we can guarantee they'll be present during upgrades. Ref: hashicorp/nomad-enterprise#1218 Ref: #18703 Ref: #18715 Ref: #16799

The RPC handlers expect to see `nil` ACL objects whenever ACLs are disabled. By using `nil` as a sentinel value, we have the risk of nil pointer exceptions and improper handling of `nil` when returned from our various auth methods that can lead to privilege escalation bugs. This is the third in a series to eliminate the use of `nil` ACLs as a sentinel value for when ACLs are disabled. This patch involves leveraging the refactored `auth` package to remove the weird "mixed auth" helper functions that only support the Variables read/list RPC handlers. Instead, pass the ACL object and claim together into the `AllowVariableOperations` method in the usual `acl` package. Ref: hashicorp/nomad-enterprise#1218 Ref: #18703 Ref: #18715 Ref: #16799 Ref: #18730 Fixes: #15875

The RPC handlers expect to see `nil` ACL objects whenever ACLs are disabled. By using `nil` as a sentinel value, we have the risk of nil pointer exceptions and improper handling of `nil` when returned from our various auth methods that can lead to privilege escalation bugs. This is the final patch in a series to eliminate the use of `nil` ACLs as a sentinel value for when ACLs are disabled. This patch adds a new virtual ACL policy field for when ACLs are disabled and updates our authentication logic to use it. Included: * Extends auth package tests to demonstrate that nil ACLs are treated as failed auth and disabled ACLs succeed auth. * Adds a new `AllowDebug` ACL check for the weird special casing we have for pprof debugging when ACLs are disabled. * Removes the remaining unexported methods (and repeated tests) from the `nomad/acl.go` file. * Update the semgrep rules to detect improper nil ACL checking and remove the old invalid ACL checks. * Update the contributing guide for RPC authentication. Ref: hashicorp/nomad-enterprise#1218 Ref: #18703 Ref: #18715 Ref: #16799 Ref: #18730 Ref: #18744

tgross added theme/auth theme/tech-debt labels Oct 10, 2023

tgross added this to the 1.7.0 milestone Oct 10, 2023

vercel bot deployed to Preview – nomad-storybook-and-ui October 10, 2023 17:42 View deployment

tgross force-pushed the no-nil-acls-server-auth branch from d04d8ae to 1299565 Compare October 10, 2023 18:34

vercel bot deployed to Preview – nomad-storybook-and-ui October 10, 2023 18:36 View deployment

tgross force-pushed the no-nil-acls-server-auth branch from 1299565 to b2d799f Compare October 10, 2023 18:56

vercel bot deployed to Preview – nomad-storybook-and-ui October 10, 2023 19:00 View deployment

tgross marked this pull request as ready for review October 10, 2023 19:21

tgross requested review from lgfa29, shoenig and jrasell October 10, 2023 19:21

jrasell approved these changes Oct 11, 2023

View reviewed changes

Apply suggestions from code review

57a829d

Co-authored-by: James Rasell <[email protected]>

vercel bot deployed to Preview – nomad-storybook-and-ui October 11, 2023 13:30 View deployment

revert slice append change to make linter happy

cc515a3

vercel bot deployed to Preview – nomad-storybook-and-ui October 11, 2023 13:43 View deployment

lgfa29 approved these changes Oct 11, 2023

View reviewed changes

tgross merged commit a92461c into main Oct 11, 2023

tgross deleted the no-nil-acls-server-auth branch October 11, 2023 14:59

tgross mentioned this pull request Oct 11, 2023

auth: add client-only ACL #18730

Merged

tgross mentioned this pull request Oct 12, 2023

auth: remove "mixed auth" special casing for Variables endpoint #18744

Merged

tgross mentioned this pull request Oct 13, 2023

auth: use ACLsDisabledACL when ACLs are disabled #18754

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

auth: add server-only ACL #18715

auth: add server-only ACL #18715

tgross commented Oct 10, 2023 •

edited

Loading

jrasell left a comment

jrasell Oct 11, 2023

tgross Oct 11, 2023

lgfa29 left a comment

lgfa29 Oct 11, 2023

tgross Oct 11, 2023

lgfa29 Oct 11, 2023

tgross Oct 11, 2023

	// ValidateCertificateForName returns true if the RPC context certificate is valid
	// for the given domain name.
	func (ctx *RPCContext) ValidateCertificateForName(name string) error {
	if ctx == nil \|\| !ctx.TLS {
	return nil
	}

	cert := ctx.Certificate()
	if cert == nil {
	return errors.New("missing certificate information")
	}

	validNames := []string{cert.Subject.CommonName}
	validNames = append(validNames, cert.DNSNames...)
	for _, valid := range validNames {
	if name == valid {
	return nil
	}
	}

	return fmt.Errorf("invalid certificate, %s not in %s", name, strings.Join(validNames, ","))
	}

auth: add server-only ACL #18715

auth: add server-only ACL #18715

Conversation

tgross commented Oct 10, 2023 • edited Loading

jrasell left a comment

Choose a reason for hiding this comment

jrasell Oct 11, 2023

Choose a reason for hiding this comment

tgross Oct 11, 2023

Choose a reason for hiding this comment

lgfa29 left a comment

Choose a reason for hiding this comment

lgfa29 Oct 11, 2023

Choose a reason for hiding this comment

tgross Oct 11, 2023

Choose a reason for hiding this comment

lgfa29 Oct 11, 2023

Choose a reason for hiding this comment

tgross Oct 11, 2023

Choose a reason for hiding this comment

tgross commented Oct 10, 2023 •

edited

Loading