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: https://github.com/hashicorp/nomad-enterprise/pull/1218 Ref: https://github.com/hashicorp/nomad/pull/18703 Ref: https://github.com/hashicorp/nomad/pull/18715 Ref: https://github.com/hashicorp/nomad/pull/16799 Ref: https://github.com/hashicorp/nomad/pull/18730 Ref: https://github.com/hashicorp/nomad/pull/18744
3.9 KiB
New/Updated RPC Endpoint Checklist
Prefer adding a new message to changing any existing RPC messages.
Code
-
Requeststruct and*RequestTypeconstant innomad/structs/structs.go. Append the constant, old constant values must remain unchanged. Just add the request type to this file, all other resource definitions must be on their own separate file. -
In
nomad/fsm.go, add a dispatch case to the switch statement in(n *nomadFSM) Apply*nomadFSMmethod to decode the request and call the state method
-
State method for modifying objects in a
Txnin thestatepackage, located innomad/state/. Every new resource should have its own file and test file, named using the conventionnomad/state/state_store_[resource].goandnomad/state/state_store_[resource]_test.go -
Handler for the request in
nomad/foo_endpoint.go- RPCs are resolved by matching the method name for bound structs net/rpc
- Register any new RPC structs in
nomad/server.go - Authentication:
- For RPCs that support HTTP APIs, call
Authenticatebefore forwarding. Return any error after frowarding, and callResolveACLto get an ACL to check. - For RPCs that support client-to-server RPCs only, use
AuthenticateClientOnlybefore forwarding. Check theAllowClientOpACL after forwarding. - For RPCs that support server-to-server RPCs only, use
AuthenticateServerOnlybefore forwarding. Check theAllowServerOpACL before forwarding.
- For RPCs that support HTTP APIs, call
- Authorization:
- Use
ResolveACLto turn the authenticated request into an ACL to check. - For Update/Get/Delete RPCs, check ACLs before hitting the state store.
- For List RPCs, use ACLs as a filter on the query.
- Never check that the ACL object is
nilto bypass authorization. The authorization methods inacl/acl.goshould already handlenilACL objects correctly (by rejecting them).
- Use
-
Wrapper for the HTTP request in
command/agent/foo_endpoint.go- Backwards compatibility requires a new endpoint, an upgraded client or server may be forwarding this request to an old server, without support for the new RPC
- RPCs triggered by an internal process may not need support
- Check ACLs as an optimization
-
Endpoint added/updated in the
nomad-openapirepository.- New endpoints will need to be configured in that repository's
generatorpackage. - Updated endpoints may require the
generatorconfiguration to change, especially if parameters or headers change. - If the accepted or returned
structschema changes, the Nomad version references ingenerator/go.modwill need to be updated. Once the version is updated, regenerate the spec and all all clients so that the new schema is reflected in the spec and thus the generated models. - If
QueryOptions,QueryMeta,WriteOptions, orWriteMetachange, thev1framework will need to updated to support the change.
- New endpoints will need to be configured in that repository's
-
nomad/core_sched.gosends many RPCsServersMeetMinimumVersionasserts that the server cluster is upgraded, so use this to guard sending the new RPC, else send the old RPC- Version must match the actual release version!
-
If implementing a Client RPC...
- Use
QueryOptionsinstead ofWriteRequestin the Request struct asWriteRequestis only for Raft writes. - Set
QueryOptions.AllowStale = truein the Server RPC forwarder to avoid an infinite loop between leaders and followers when a Client RPC is forwarded through a follower. See https://github.com/hashicorp/nomad/issues/16517
- Use